FIX: Performance optimisation for draft pages in treeview #181

sminnee · 2018-09-17T22:42:13Z

When draft pages are rendered in the treeview, an attempt to look up
their live Version # is made. This leads to a cache miss and a
subsequent query for each record.

This change marks a cache as “complete” when all records of a given
stage are pre-cached. This will suppress the record-specific query
in the case of a cache miss.

Parent issue

/treeview endpoint is slow when a lot of nodes are in the tree silverstripe-cms#2250

When draft pages are rendered in the treeview, an attempt to look up their live Version # is made. This leads to a cache miss and a subsequent query for each record. This change marks a cache as “complete” when all records of a given stage are pre-cached. This will suppress the record-specific query in the case of a cache miss.

chillu · 2018-09-17T22:53:25Z

Note that unless this fix is considered "critical" and rebased against the 1.2 branch, it won't be available for the targeted project (since they're trying to avoid a 4.3 recipe upgrade). I'd say it's non-critical, and has a moderate chance of functional regressions, so it's targeted correctly with 1. But that won't help the targeted project which it has been written for.

maxime-rainville · 2018-09-18T02:14:55Z

I tested on our key project. It doesn't look like it makes much of a difference. I've compared building the treeview for the North Island page in black fire.

This is comparing the original version to the patched version.

We save 245 queries ( out of 2 381 ) and improve performance time by about 2.2%.

@chillu I can review this in greater details if you want, but it's not going to resolve our treeview issues for our key project.

chillu · 2018-09-18T02:43:28Z

Assuming this is the PR which reduces number of queries, it'll have a far bigger impact in SSP with a remote database than on your local. No need to set up Blackfire on SSP for this, but I think it's still worthwhile to get peer reviewed and merged this week.

sminnee · 2018-09-18T04:08:41Z

Note that this requires unpublished draft page to have much of an effect.

maxime-rainville

Looks good and it should be functionally equivalent.

I've added some unit test to improve code coverage.

robbieaverill · 2018-09-19T09:32:53Z

tests/php/VersionedTest.php

+        $this->assertNull(
+            Versioned::get_versionnumber_by_stage(VersionedTest\TestObject::class, Versioned::DRAFT, 888, true),
+            'get_versionnumber_by_stage should return null when fetching a version number for unexisting object with prepopulate_versionnumber_caching'
+        );


This test is actually six or seven different tests, or different scenarios for one test. Can you please split it up or use a data provider?

What about I split it in 4:

1 for getting an existing version number that is uncached and cached (2 assertions)

1 for getting an non-existing version number without the cache flag (1 assertion)

1 for getting an non-existing version number that is uncached and cached (2 assertions)

1 for getting an non-existing version number after a call to prepopulate_versionnumber_cache (1 assertion)

I'd rather avoid splitting the uncached/cached assertions.

Given Robbie is only available in EU timezone, I'll say: Yep, that sounds good ;)

Sorry yeah sounds good

Sounds fine to me too, although you might find it easier with 6 separate tests using a data provider. And it's probably worth it if you think each of the assertions could fail in isolation.

I've switched to using a data provider and use a more thorough parameter coverage. I've moved the logic to a separate file because I need to have some setUp and setUpBeforeClass customisation.

The new tests actually found a bug, so I fix that as well.

Heh its funny, I've just commented on how unreadable the data provider refactor made this code ;) But I'd say it's good enough to merge

Move VersionedNumber caching to their own test suite and tweak them to use a data provider. Also, fix a bug when trying to bypass the completed cache and fetch an non existing version number.

tractorcow

Smart idea good job.

ScopeyNZ · 2018-09-21T01:54:03Z

src/Versioned.php

+        if ($cache) {
+            if (isset(self::$cache_versionnumber[$baseClass][$stage][$id])) {
+                return self::$cache_versionnumber[$baseClass][$stage][$id] ?: null;
+            } elseif (isset(self::$cache_versionnumber[$baseClass][$stage]['_complete'])) {


This can technically be just an if as you're returning in the preceding if statement. Not a blocker though - super super minor.

I think, I'll keep it as-is just to make it clear the second bit will only run if the first if statement is false.

sminnee · 2018-09-21T03:48:09Z

OK this looks like we've addressed Robbie's feedback. Since I wrote this does @chillu want to merge?

chillu · 2018-09-21T04:01:11Z

tests/php/VersionedNumberCacheTest.php

+
+    public function cacheDataProvider()
+    {
+        return [


As a sidenote, I find that data providers should only be used if the data within them is pretty self explanatory. If you find yourself going back and forth between the provider and the implementation (like I'm doing in this case), it's not helping readability. And in unit tests, I think readability overrules DRY principles.

@robbieaverill @chillu You guys are tough customers to please 😉

I'll keep it in mind for the next time.

chillu requested a review from maxime-rainville September 17, 2018 22:43

sminnee mentioned this pull request Sep 17, 2018

Epic: CMS performance improvements silverstripe/silverstripe-admin#571

Open

2 tasks

sminnee mentioned this pull request Sep 17, 2018

Precache Hierarchy::numChildren() for performance silverstripe/silverstripe-framework#8379

Closed

sminnee requested a review from tractorcow September 17, 2018 23:43

chillu assigned maxime-rainville Sep 18, 2018

Improve unit test converage for prepopulate_versionnumber_cache.

02a201e

maxime-rainville approved these changes Sep 19, 2018

View reviewed changes

robbieaverill suggested changes Sep 19, 2018

View reviewed changes

Use Data provider to test version number caching

840f008

Move VersionedNumber caching to their own test suite and tweak them to use a data provider. Also, fix a bug when trying to bypass the completed cache and fetch an non existing version number.

tractorcow approved these changes Sep 21, 2018

View reviewed changes

ScopeyNZ approved these changes Sep 21, 2018

View reviewed changes

Switch back to using VersionedTest\TestObject

651da34

chillu reviewed Sep 21, 2018

View reviewed changes

chillu merged commit e44954d into silverstripe:1 Sep 21, 2018

sminnee mentioned this pull request Sep 28, 2018

Generate more realistic tree shapes in sample data silverstripe/silverstripe-frameworktest#54

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Performance optimisation for draft pages in treeview #181

FIX: Performance optimisation for draft pages in treeview #181

sminnee commented Sep 17, 2018 •

edited by maxime-rainville

Loading

chillu commented Sep 17, 2018

maxime-rainville commented Sep 18, 2018

chillu commented Sep 18, 2018

sminnee commented Sep 18, 2018

maxime-rainville left a comment

robbieaverill Sep 19, 2018

maxime-rainville Sep 19, 2018

chillu Sep 20, 2018

robbieaverill Sep 20, 2018

ScopeyNZ Sep 20, 2018 •

edited

Loading

maxime-rainville Sep 21, 2018

chillu Sep 21, 2018

tractorcow left a comment

ScopeyNZ Sep 21, 2018

maxime-rainville Sep 21, 2018

sminnee commented Sep 21, 2018

chillu Sep 21, 2018

maxime-rainville Sep 23, 2018

FIX: Performance optimisation for draft pages in treeview #181

FIX: Performance optimisation for draft pages in treeview #181

Conversation

sminnee commented Sep 17, 2018 • edited by maxime-rainville Loading

Parent issue

chillu commented Sep 17, 2018

maxime-rainville commented Sep 18, 2018

chillu commented Sep 18, 2018

sminnee commented Sep 18, 2018

maxime-rainville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScopeyNZ Sep 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tractorcow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sminnee commented Sep 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sminnee commented Sep 17, 2018 •

edited by maxime-rainville

Loading

ScopeyNZ Sep 20, 2018 •

edited

Loading